Add union to opaque comparisons #8896
Conversation
friendlymatthew
force-pushed
the
friendlymatthew/union-to-opaque
branch
from
5f9ca9b to
35ec44d
Compare
|
cc @alamb (sorry for the constant pinging I feel) |
|
No problem @friendlymatthew -- I'll put it on my review queue I am currently putting my arrow-rs focus on getting the release out. I'll probably have more time for reviews next week It looks like there are several CI failures with this PR |
friendlymatthew
force-pushed
the
friendlymatthew/union-to-opaque
branch
2 times, most recently
from
e8a2b1e to
9b30e9a
Compare
friendlymatthew
force-pushed
the
friendlymatthew/union-to-opaque
branch
from
9b30e9a to
2e45dce
Compare
|
No problem for the pings -- sorry for the delay -- I was focusing on getting the arrow releases out and then vacation/picking up DataFusion reviews. I now have some more bandwidth for arrow-r |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you for this PR @friendlymatthew -- I am not sure about this one. Sorry if I mislead you earlier
| unreachable!() | ||
| }; | ||
|
|
||
| let opaque_type_id = union_fields |
There was a problem hiding this comment.
I am not sure about this -- it seems like it will compare the union array to the first field that has the same type.
What if the union array has multiple repeated types (I realize that is not a common use case, but I think it is possible)
| }, | ||
| (Map(_, _), Map(_, _)) => compare_map(left, right, opts), | ||
| (Union(_, _), Union(_, _)) => compare_union(left, right, opts), | ||
| (Union(_, _), _) => compare_union_to_opaque(left, right, opts), |
There was a problem hiding this comment.
Is there any other example of comparing one array to an array of another type? I think this kernel expects the types to match exactly
I do see the argument about how UnionArray could be treated as a special case (as some sort of runtime typed thing) 🤔 but I am not sure UnionArrays are required to behave that way
I wrote up another way that your usecase might be handled
Let me know what you think
2 participants
Which issue does this PR close?
Rationale for this change
This PR implements comparison logic for Union arrays against opaque (non-union) arrays in the
ordkernelWhen comparing a union to an opaque array, the comparison first checks if the union's type id matches the opaque array's type, then compares the actual values if types align. This provides a consistent ordering where union variants are ordered by their type id relative to the opaque type
Notes about this PR
Please review from the second commit. The first commit being #8838